testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java (1)

70-85: ⚡ Quick win

Add a regression test and a CHANGES.txt entry for this flaky-test fix.

The PR description acknowledges that tests and CHANGES.txt were not updated. For a fix targeted at a flaky test (issue #2586, related to #599), a deterministic regression test that exercises the timeout/invocationCount path and asserts exactly one increment of getCurrentInvocationCount() (and one passed result in verifyTests()) would prevent future regressions of the same class of bug — particularly given that the counter-increment semantics under timeouts are now subtly different from the previous behavior. A short note in CHANGES.txt referencing #2586 is also worth adding for downstream consumers.

Want me to draft a regression test (e.g. a small HookableTest-style class with invocationCount + invocationTimeOut plus a verifier on getCurrentInvocationCount()), and/or a CHANGES.txt line referencing #2586?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java`
around lines 70 - 85, Add a deterministic regression test and a CHANGES.txt
entry to prevent regressions around the timeout+invocationCount behavior fixed
in InvokeMethodRunnable.call(): create a small HookableTest-like test that sets
a method with invocationCount > 1 and invocationTimeOut > 0, exercise the
timeout path so only one run completes, assert that
m_method.getCurrentInvocationCount() == 1 (and that verifyTests() reports one
passed result), and ensure the test reliably reproduces the previous flaky
behavior; also add a short CHANGES.txt line referencing issue `#2586` describing
the flaky-test fix so downstream consumers are aware.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java (1)

52-59: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid re-wrapping TestNGRuntimeException in run()

runOne() now throws TestNGRuntimeException, but run() catches Exception and wraps it again, producing nested TestNGRuntimeException and obscuring the root cause chain.

Proposed fix

 public boolean run() throws TestNGRuntimeException {
   try {
     return call();
+  } catch (TestNGRuntimeException e) {
+    throw e;
   } catch (Exception e) {
     throw new TestNGRuntimeException(e);
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java`
around lines 52 - 59, The catch in InvokeMethodRunnable.run() re-wraps
exceptions from runOne() into another TestNGRuntimeException; update the catch
logic so that if the caught Throwable (or its cause) is already a
TestNGRuntimeException you rethrow it as-is instead of creating a new wrapper:
inspect e (and e.getCause()), if instanceof TestNGRuntimeException assign that
instance to t (or throw it directly), otherwise wrap the root cause in a new
TestNGRuntimeException as currently done; ensure this change is applied in the
catch block that sets invoked and t so run() does not produce nested
TestNGRuntimeException instances.